Skip to content

Fix git-sync rotated credentials support in Helm chart#63276

Open
sidshas03 wants to merge 4 commits intoapache:mainfrom
sidshas03:issue-63253-gitsync-password-file
Open

Fix git-sync rotated credentials support in Helm chart#63276
sidshas03 wants to merge 4 commits intoapache:mainfrom
sidshas03:issue-63253-gitsync-password-file

Conversation

@sidshas03
Copy link
Contributor

Fixes #63253

Hi team, this PR is to handle token rotation properly when dags.gitSync.credentialsSecret is used.

Problem

Right now password/token is passed as env var (GIT_SYNC_PASSWORD / GITSYNC_PASSWORD).
When secret is rotated (for example ESO GitHub App token), running git-sync container does not pick the new value until restart.

What I changed

  • Added secret volume mount for git-sync credentials (git-sync-credentials) in:
    • scheduler
    • worker
    • triggerer
    • dag-processor
  • Switched password config to file-based envs:
    • GIT_SYNC_PASSWORD_FILE=/etc/git-secret/credentials/GIT_SYNC_PASSWORD
    • GITSYNC_PASSWORD_FILE=/etc/git-secret/credentials/GITSYNC_PASSWORD
  • Kept username keys from secret as before (GIT_SYNC_USERNAME / GITSYNC_USERNAME).
  • Updated chart values comments + schema description.
  • Added/updated helm tests for scheduler/worker/triggerer/dag-processor.

Why this fix

git-sync can re-read password from file path on sync loop, so rotated token is picked without forcing pod restart.

Testing

Ran helm unit tests for the updated git-sync paths and related suites (HELM_TEST_KUBERNETES_VERSION=1.32.8), and selected tests are passing.

  1. Ran focused Helm unit tests for the new credential-file behavior:

    • test_git_sync_scheduler.py::test_should_set_username_and_password_file_env_variables
    • test_git_sync_worker.py::test_should_set_password_file_env_variables_when_credentials_secret_is_configured
    • test_git_sync_triggerer.py::test_should_set_password_file_env_variables_when_credentials_secret_is_configured
    • test_dag_processor.py::test_should_set_password_file_env_variables_when_credentials_secret_is_configured
      Result: 4 passed
  2. Ran broader git-sync regression subset:

    • Full files:
      • helm-tests/tests/helm_tests/other/test_git_sync_scheduler.py
      • helm-tests/tests/helm_tests/other/test_git_sync_worker.py
      • helm-tests/tests/helm_tests/other/test_git_sync_triggerer.py
    • Plus dag-processor git-sync checks:
      • test_validate_if_ssh_params_are_added_with_git_ssh_key
      • test_should_set_password_file_env_variables_when_credentials_secret_is_configured
        Result: 42 passed
  3. Sanity checks:

    • jq empty chart/values.schema.json (schema JSON valid)
    • Python compile check for modified tests (compileall) passed

Environment note: tests were run with HELM_TEST_KUBERNETES_VERSION=1.32.8.

@electrical
Copy link

@sidshas03 Airflow currently uses version 4.4.2 of gitsync, according to kubernetes/git-sync#976 the feature required is in 4.6.0?
I could be missing something :)

@sidshas03
Copy link
Contributor Author

@sidshas03 Airflow currently uses version 4.4.2 of gitsync, according to kubernetes/git-sync#976 the feature required is in 4.6.0? I could be missing something :)

Good catch, thanks for pointing this out. You’re right, in git-sync v4.4.2, --password-file exists but it is not re-read on every sync. The auto re-read for rotated credentials came in v4.6.0 via kubernetes/git-sync#976. So to make this fix effective by default, we should run git-sync >= v4.6.0.

Copy link
Contributor

@Miretpl Miretpl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall - good idea, but we cannot break backward compatibility for Git-Sync (we still support version 3 even). I placed some idea how to do it in the comments.

Also, Git-Sync container is used in pod-template-file too (used by KubernetesExecutor). Could you add a change there too?

{{- if or .Values.dags.gitSync.sshKeySecret .Values.dags.gitSync.sshKey}}
{{- include "git_sync_ssh_key_volume" . | indent 8 }}
{{- end }}
{{- if .Values.dags.gitSync.credentialsSecret }}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
{{- if .Values.dags.gitSync.credentialsSecret }}
{{- if and $localOrDagProcessorDisabled .Values.dags.gitSync.enabled .Values.dags.gitSync.credentialsSecret }}

following git sync container falgs for scheduler

{{- if or .Values.dags.gitSync.sshKeySecret .Values.dags.gitSync.sshKey}}
{{- include "git_sync_ssh_key_volume" . | nindent 8 }}
{{- end }}
{{- if .Values.dags.gitSync.credentialsSecret }}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
{{- if .Values.dags.gitSync.credentialsSecret }}
{{- if and .Values.dags.gitSync.enabled (not .Values.dags.persistence.enabled) }}

following git sync container falgs for triggerer

{{- if or .Values.dags.gitSync.sshKeySecret .Values.dags.gitSync.sshKey}}
{{- include "git_sync_ssh_key_volume" . | indent 8 }}
{{- end }}
{{- if .Values.dags.gitSync.credentialsSecret }}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
{{- if .Values.dags.gitSync.credentialsSecret }}
{{- if and .Values.dags.gitSync.enabled (not .Values.dags.persistence.enabled) }}

following git sync container falgs for workers

defaultMode: 288
{{- end }}

{{/* Git credentials volume */}}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
{{/* Git credentials volume */}}
{{/* Git credentials volume */}}

Comment on lines -250 to +261
- name: GIT_SYNC_PASSWORD
valueFrom:
secretKeyRef:
name: {{ .Values.dags.gitSync.credentialsSecret | quote }}
key: GIT_SYNC_PASSWORD
- name: GITSYNC_PASSWORD
valueFrom:
secretKeyRef:
name: {{ .Values.dags.gitSync.credentialsSecret | quote }}
key: GITSYNC_PASSWORD
- name: GIT_SYNC_PASSWORD_FILE
value: "/etc/git-secret/credentials/GIT_SYNC_PASSWORD"
- name: GITSYNC_PASSWORD_FILE
value: "/etc/git-secret/credentials/GITSYNC_PASSWORD"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change breaks backward-compatibility. You could prevent that by making if condition on these envs where, by default, old behaviour will be in place.

"description": "The gitSync image tag.",
"type": "string",
"default": "v4.4.2"
"default": "v4.6.0"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this should be in a separate PR as it is unrelated change for this one.

Comment on lines +10803 to +10804
"credentialsSecret": {
"description": "Name of a Secret containing the repo `GIT_SYNC_USERNAME` and `GIT_SYNC_PASSWORD`.",
"description": "Name of a Secret containing `GIT_SYNC_USERNAME`, `GITSYNC_USERNAME`, `GIT_SYNC_PASSWORD`, and `GITSYNC_PASSWORD` keys. The password keys are mounted as files and used via `*_PASSWORD_FILE` env vars so rotated credentials can be re-read.",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You probably need to create a new field dedicated for the file as this breaks backward-compatibility.

@sidshas03
Copy link
Contributor Author

sidshas03 commented Mar 10, 2026

Thanks @Miretpl for the detailed review. I got your point on the backward compatibility part, and I will rework this accordingly. I will keep the existing credentialsSecret behaviour as it is, and instead add a separate field for the file based password support, so the current contract does not change. I will also remove the git-sync image tag bump from this PR and take it separately. And yes, I will add the corresponding change in the pod-template-file path used by KubernetesExecutor as well.

@jscheffl
Copy link
Contributor

Note: We are currently planning and discussing a revamt to a chart v2 where we want to re-model some things and also drop some legacy. Would it be OK to "park" this PR, drop all git sync legacy support and use minimum 4.6.0 from then on and have this as a new feature? w/o all the backwards compatatibility complexity?

@sidshas03
Copy link
Contributor Author

Thanks for the guidance @jscheffl. I’m parking this PR as discussed. I’m interested to work on the chart v2 follow-up (git-sync >= 4.6.0) as well, please tag me when that issue/epic is created.

@jscheffl jscheffl added this to the Airflow Helm Chart 2.0.0 milestone Mar 10, 2026
@electrical
Copy link

@jscheffl Do you have an estimation on the timeframe for the new Helm chart?
The current issue that this PR solves is not a major problem but would be nice to have it resolved to avoid crashing containers all the time :)

@jscheffl
Copy link
Contributor

@electrical We had initial discussions, are currently collecting topics and will drop a DISCUSS to devlist once we have a (draft) plan.

@sidshas03 sidshas03 closed this Mar 11, 2026
@sidshas03 sidshas03 reopened this Mar 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:helm-chart Airflow Helm Chart

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Gitsync fails to read new credentials when using ESO Github token generator

4 participants